(graphcache) - Add mergeMode to simplePagination helper #1174

hoangvvo · 2020-11-19T07:10:03Z

Resolves #1173

Summary

Set of changes

Adds mergeMode to simplePagination similar to that of relayPagination. Default to inwards, so it will not be a breaking change.

changeset-bot · 2020-11-19T07:10:06Z

🦋 Changeset detected

Latest commit: 6f1231a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@urql/exchange-graphcache	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

JoviDeCroock

Looks good! Thank you for taking the time to PR this, mind adding a changeset by running yarn changeset at the root? This would be a minor for @urql/exchange-graphcache since we're adding a pagination feature! 👍

exchanges/graphcache/src/extras/simplePagination.ts

Co-authored-by: Jovi De Croock <[email protected]>

hoangvvo · 2020-11-19T07:56:58Z

Thanks for such a quick review! I added documentation for mergeMode of Simple Pagination based on that of Relay Pagination. Changeset also added.

kitten · 2020-11-19T13:35:34Z

I believe this is misunderstanding what the mergeMode does on Relay pagination.
There it takes care of which order things are merged in, yes, but in relation to how the pagination arguments are used, since before and after cursors can be used for the same list. This then creates ambiguity because both directions can be "outward" from the same point or "inward" towards a shared centre in the list.

I believe mergeMode may not make much sense for simplePagination as we're not dealing with two directions of pagination so inwards/outwards aren't descriptive.

Furthermore I'm unsure the explanation in the RFC is accurate and whether a lot of APIs even use this pattern? Do we have some prior art here?

hoangvvo · 2020-11-19T15:24:31Z

@kitten You are right. I am not familiar with Relay and possibly misusing the term inwards and outwards. Either way, here are the relevant words from relay pagination definition:

Outwards pagination assumes that pages
that come in last should be merged before the first pages, [...] The default inwards pagination assumes that
pagination last pages is part of the same list and come after first pages.

This matches the description of what I am trying to achieve. Perhaps, mergeMode = "before" | "after" or mergeMode = "reverse" | "forward" might be better. It satisfies use-cases of infinite scroll but instead of scrolling down for older items, we scroll up for them. The GIF above demonstrates that.

kitten · 2020-11-19T15:27:02Z

@hoangvvo i believe would rename it like that 👍 the reason why the description matches is because for this paragraph it describes rather what happens when we start pagination from in reverse while forward pagination has been used and vice-versa. So the behaviour does match but it's for a subtly different reason 😅

hoangvvo · 2020-11-19T15:45:03Z

@hoangvvo i believe would rename it like that the reason why the description matches is because for this paragraph it describes rather what happens when we start pagination from in reverse while forward pagination has been used and vice-versa. So the behaviour does match but it's for a subtly different reason

I agree that it is still a somewhat uncommon pattern. I can definitely workaround this by simply reverse the returned GraphQL array appropriately.

offset = 0 => 9,8,7
offset = 3 => 6,5,4

The current simplePagination will merge it into [9,8,7,6,5,4] and I would only need to reverse it to achieve the above effect. So it is okay to not putting this into the helper.

Otherwise, how would you rename it? I am steering toward mergeMode = "before" | "after" with the default being after, where pages with higher offset will be put after, and can otherwise be "before" where pages with higher offset will be put before. The doc will also be updated to reflect this instead of using the terms inwards and outwards:

offset = 0 => 7,8,9
offset = 3 => 4,5,6

mergeMode = "before" => [4,5,6,  7,8,9]
mergeMode = "after" => [7,8,9,  4,5,6]
```

hoangvvo · 2020-11-19T15:57:12Z

@kitten I updated the RFC to clarify the intention and approach

kitten · 2020-11-19T16:11:48Z

Awesome! Yea, I think 'before' | 'after' is definitely the most intuitive solution 👍

kitten

There's one more section in the docs that needs updating, but I can totally do that after this PR myself if you'd like ✌️ https://github.com/FormidableLabs/urql/blob/main/docs/api/graphcache.md#simplepagination

Thanks for adding this!

kitten · 2020-11-20T08:51:00Z

.changeset/little-crabs-sell.md

+'@urql/exchange-graphcache': minor
+---
+
+Add mergeMode = 'before' | 'after' to simplePagination helper to define whether pages are merged before or after when you paginate forwards and backwards


Suggested change

Add mergeMode = 'before' | 'after' to simplePagination helper to define whether pages are merged before or after when you paginate forwards and backwards

Add a `mergeMode: 'before' | 'after'` option to the `simplePagination` helper to define whether pages are merged before or after preceding ones when pagination, similar to `relayPagination`'s option

Just suggesting some details here since we can use full markdown in our changelog 😇

kitten · 2020-11-20T08:53:28Z

docs/graphcache/computed-queries.md

+You can also specify `mergeMode`, which defaults to `after` and can otherwise
+be set to `before`. This will handle how pages are merged when you paginate


Suggested change

You can also specify `mergeMode`, which defaults to `after` and can otherwise

be set to `before`. This will handle how pages are merged when you paginate

We may also add the `mergeMode` option, which defaults to `'after'` and can otherwise

be set to `'before'`. This will handle in which order pages are merged when paginating

We usually use "we" as often as possible when writing (with some exceptions). I realise the Graphcache pages may need some revamping to make sure they're as helpful as can be compared to some others, but let's take that into account here

hoangvvo · 2020-11-20T13:53:23Z

Thanks for the review, I updated the documentation and also added to graphcache.md.

Add mergeMode to Simple Pagination

eb57a43

hoangvvo mentioned this pull request Nov 19, 2020

Add Persistent Messages auralous/auralous#22

Merged

JoviDeCroock reviewed Nov 19, 2020

View reviewed changes

exchanges/graphcache/src/extras/simplePagination.ts Outdated Show resolved Hide resolved

hoangvvo and others added 3 commits November 19, 2020 02:43

Add documentation for Simple Pagination mergeMode

48b1f1f

Add changeset

9472c33

Update exchanges/graphcache/src/extras/simplePagination.ts

7b9b54b

Co-authored-by: Jovi De Croock <[email protected]>

hoangvvo force-pushed the feat/simplePagination-mergeMode branch from 46a4b51 to 7b9b54b Compare November 19, 2020 07:54

hoangvvo added 2 commits November 19, 2020 12:01

Change mergeMode to before | after

ab24de4

Update Changelog

155eee6

kitten approved these changes Nov 20, 2020

View reviewed changes

hoangvvo added 3 commits November 20, 2020 08:40

Update changeset

8985c95

Update Computed Queries documentation

c0ef225

Add mergeMode option to graphcache documentation

6f1231a

kitten merged commit 2f8db84 into urql-graphql:main Nov 20, 2020

github-actions bot mentioned this pull request Nov 20, 2020

Version Packages #1170

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(graphcache) - Add mergeMode to simplePagination helper #1174

(graphcache) - Add mergeMode to simplePagination helper #1174

hoangvvo commented Nov 19, 2020

changeset-bot bot commented Nov 19, 2020 •

edited

Loading

JoviDeCroock left a comment

hoangvvo commented Nov 19, 2020

kitten commented Nov 19, 2020

hoangvvo commented Nov 19, 2020

kitten commented Nov 19, 2020

hoangvvo commented Nov 19, 2020 •

edited

Loading

hoangvvo commented Nov 19, 2020

kitten commented Nov 19, 2020

kitten left a comment

kitten Nov 20, 2020

kitten Nov 20, 2020

hoangvvo commented Nov 20, 2020

	Add mergeMode = 'before' \| 'after' to simplePagination helper to define whether pages are merged before or after when you paginate forwards and backwards
	Add a `mergeMode: 'before' \| 'after'` option to the `simplePagination` helper to define whether pages are merged before or after preceding ones when pagination, similar to `relayPagination`'s option

		You can also specify `mergeMode`, which defaults to `after` and can otherwise
		be set to `before`. This will handle how pages are merged when you paginate

(graphcache) - Add mergeMode to simplePagination helper #1174

(graphcache) - Add mergeMode to simplePagination helper #1174

Conversation

hoangvvo commented Nov 19, 2020

Summary

Set of changes

changeset-bot bot commented Nov 19, 2020 • edited Loading

🦋 Changeset detected

JoviDeCroock left a comment

Choose a reason for hiding this comment

hoangvvo commented Nov 19, 2020

kitten commented Nov 19, 2020

hoangvvo commented Nov 19, 2020

kitten commented Nov 19, 2020

hoangvvo commented Nov 19, 2020 • edited Loading

hoangvvo commented Nov 19, 2020

kitten commented Nov 19, 2020

kitten left a comment

Choose a reason for hiding this comment

kitten Nov 20, 2020

Choose a reason for hiding this comment

kitten Nov 20, 2020

Choose a reason for hiding this comment

hoangvvo commented Nov 20, 2020

changeset-bot bot commented Nov 19, 2020 •

edited

Loading

hoangvvo commented Nov 19, 2020 •

edited

Loading